Skip to content

[ENG-581] Optimize complete service request logic by remove duplication of mutation#16462

Open
NikhilA8606 wants to merge 1 commit into
developfrom
ENG-581-optimize-complete-service-request-logic-by-remove-duplication-of-mutation
Open

[ENG-581] Optimize complete service request logic by remove duplication of mutation#16462
NikhilA8606 wants to merge 1 commit into
developfrom
ENG-581-optimize-complete-service-request-logic-by-remove-duplication-of-mutation

Conversation

@NikhilA8606

@NikhilA8606 NikhilA8606 commented Jun 18, 2026

Copy link
Copy Markdown
Member

The "complete service request" API was being defined in two places once in ServiceRequestShow and again inside CompletionNoteContent but only the child's copy was actually used. Removed the duplicate mutation from the parent and kept a single definition inside CompletionNoteContent
ENG-581

Tagging: @ohcnetwork/care-fe-code-reviewers

Merge Checklist

  • Add specs that demonstrate the bug or test the new feature.
  • Update product documentation.
  • Ensure that UI text is placed in I18n files.
  • Prepare a screenshot or demo video for the changelog entry and attach it to the issue.
  • Request peer reviews.
  • Complete QA on mobile devices.
  • Complete QA on desktop devices.
  • Add or update Playwright tests for related changes

Summary by CodeRabbit

  • Refactor
    • Restructured the service request completion workflow to streamline dialog interaction patterns.

@NikhilA8606 NikhilA8606 requested review from a team and Copilot June 18, 2026 06:32
@netlify

netlify Bot commented Jun 18, 2026

Copy link
Copy Markdown

Deploy Preview for care-ohc ready!

Name Link
🔨 Latest commit b3add71
🔍 Latest deploy log https://app.netlify.com/projects/care-ohc/deploys/6a33910dc1c3240008847e92
😎 Deploy Preview https://deploy-preview-16462.preview.ohc.network
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai

coderabbitai Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

ServiceRequestShow no longer owns the completionNote state or the completeServiceRequest mutation. Both are moved into CompletionNoteContent, which receives facilityId, serviceRequestId, serviceRequest, setIsCompleteDialogOpen, and onCancel as props and manages the full completion flow internally.

Changes

CompletionNoteContent self-containment refactor

Layer / File(s) Summary
Remove parent-level state and mutation
src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx
Deletes completionNote state and the parent-level completeServiceRequest mutation hook. Simplifies Sheet/Dialog onOpenChange handlers to call setIsCompleteDialogOpen directly, removing the previous guard on mutation pending state. Adds ServiceRequestReadSpec to imports for child component typing.
CompletionNoteContent: new props contract and internal mutation
src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx
Replaces controlled-note props (note, isUpdating, onNoteChange, onComplete) with facilityId, serviceRequestId, serviceRequest, setIsCompleteDialogOpen, and onCancel. Adds internal note state initialized from serviceRequest.note, a useMutation call to serviceRequestApi.updateServiceRequest, and on-success query invalidation, toast, and dialog close. Binds textarea and button disabled states to the internal mutation's pending flag.

Possibly related PRs

  • ohcnetwork/care_fe#15837: Directly related — introduced CompletionNoteContent, the completion dialog/sheet, and the initial serviceRequestApi.updateServiceRequest wiring that this PR refactors.

Suggested labels

needs review, needs peer review

Suggested reviewers

  • amjithtitus09
  • Jacobjeevan
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description check ✅ Passed The description explains the issue and solution clearly, follows the template structure, and includes issue tracking. However, the 'Proposed Changes' section lacks a 'Fixes #issue_number' reference and detailed bullet points.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title matches the PR’s main refactor: removing duplicate service request completion mutation logic.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ENG-581-optimize-complete-service-request-logic-by-remove-duplication-of-mutation

Comment @coderabbitai help to get the list of available commands.

@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying care-preview with  Cloudflare Pages  Cloudflare Pages

Latest commit: b3add71
Status: ✅  Deploy successful!
Preview URL: https://d29b9a50.care-preview-a7w.pages.dev
Branch Preview URL: https://eng-581-optimize-complete-se.care-preview-a7w.pages.dev

View logs

@greptile-apps

greptile-apps Bot commented Jun 18, 2026

Copy link
Copy Markdown

Greptile Summary

This PR consolidates the completeServiceRequest mutation, previously duplicated in ServiceRequestShow and CompletionNoteContent, into a single definition inside the child component. The refactor also moves the note state and its initialization into CompletionNoteContent.

  • The mutation de-duplication and prop interface cleanup are correct; CompletionNoteContent now owns all completion logic and the parent no longer needs to coordinate mutation state.
  • The onOpenChange handlers for both Sheet and Dialog were simplified from a guard (if (!isCompletingServiceRequest) setIsCompleteDialogOpen(open)) to a bare setIsCompleteDialogOpen, removing the protection against backdrop-click or Escape-key dismissal while a submission is in flight.

Confidence Score: 4/5

Safe to merge with low risk; the mutation itself still completes correctly, but a UX regression was introduced around dialog dismissal during submission.

The consolidation is sound and the mutation logic is unchanged. The one regression is that the backdrop-click / Escape-key lock that prevented users from accidentally closing the dialog mid-submission is no longer in effect, since isCompletingServiceRequest is no longer accessible to the parent's onOpenChange handlers.

src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx — specifically the onOpenChange prop on both the Sheet and Dialog wrappers.

Important Files Changed

Filename Overview
src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx Moves the completeServiceRequest mutation and related state from the parent into CompletionNoteContent. The refactoring is correct and reduces duplication, but the guard that blocked backdrop/Escape-key dialog dismissal while the mutation was pending was dropped and not re-implemented.

Reviews (1): Last reviewed commit: "ENG-581 Optimize complete service reques..." | Re-trigger Greptile

Comment thread src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the “mark as complete” flow in the Service Request details page by consolidating the “complete service request” mutation into CompletionNoteContent, removing the previously duplicated parent-level mutation/state.

Changes:

  • Removed parent-managed completion note state and completion mutation from ServiceRequestShow.
  • Moved completion mutation and note state into CompletionNoteContent, passing facilityId, serviceRequestId, and serviceRequest as inputs.
  • Simplified Dialog/Sheet open/close handling by wiring onOpenChange directly to setIsCompleteDialogOpen.

Comment thread src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx
Comment thread src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx (3)

707-745: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Document this medical data update path.

This mutation completes a service request and persists the completion note/location IDs; add a short data-flow and validation-boundary note next to the mutation/payload.

As per coding guidelines, src/pages/**/*.{ts,tsx} page components must document medical data flows and validation requirements.

Suggested documentation
+  /**
+   * Medical data flow:
+   * - completes the service request via `serviceRequestApi.updateServiceRequest`
+   * - persists a trimmed completion note, or `null` when empty
+   * - preserves current service-request location IDs from the loaded request
+   *
+   * Validation boundary: this path relies on the update API/backend to validate
+   * allowed status transitions and location membership.
+   */
   const {
     mutate: completeServiceRequest,
     isPending: isCompletingServiceRequest,
   } = useMutation({
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx` around
lines 707 - 745, Add documentation above the useMutation hook for
completeServiceRequest that explains the medical data flow and validation
boundaries. Include a comment that describes what medical data is being
persisted (the completion note and location IDs), the validation that occurs
(such as the note being trimmed), and the data-flow path from the form input
through the mutation to the backend update. This follows the coding guideline
that page components in src/pages/**/*.{ts,tsx} must document medical data flows
and validation requirements.

Source: Coding guidelines


688-762: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Extract CompletionNoteContent into its own component file.

Now that this component owns state, mutation, and side effects, move it to components/CompletionNoteContent.tsx with a default export. While extracting, prefer a single onClose callback instead of passing both onCancel and setIsCompleteDialogOpen.

As per coding guidelines, TSX React components should prefer one component per file, default exports, and ComponentName.tsx naming.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx` around
lines 688 - 762, Extract the CompletionNoteContent component from
ServiceRequestShow.tsx into a new file at components/CompletionNoteContent.tsx
as a default export. Update the CompletionNoteContentProps interface to replace
the two separate callbacks onCancel and setIsCompleteDialogOpen with a single
onClose callback. Update all usages of these two callbacks within the component
to call onClose instead. This consolidation simplifies the component's interface
and follows the one-component-per-file convention with default exports as per
the coding guidelines.

Source: Coding guidelines


640-663: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Block Sheet/Dialog closure while completion is pending.

onOpenChange={setIsCompleteDialogOpen} lets outside-click/Escape/close events dismiss the completion surface while the child-owned mutation is still in flight. Since the pending state now lives in CompletionNoteContent, propagate that state upward or move the open wrapper into the child so the close guard still applies.

Suggested direction
   const [isCompleteDialogOpen, setIsCompleteDialogOpen] = useState(false);
+  const [isCompletingServiceRequest, setIsCompletingServiceRequest] =
+    useState(false);

 ...
             <Sheet
               open={isCompleteDialogOpen}
-              onOpenChange={setIsCompleteDialogOpen}
+              onOpenChange={(open) => {
+                if (!isCompletingServiceRequest) setIsCompleteDialogOpen(open);
+              }}
             >
 ...
                 <CompletionNoteContent
                   facilityId={facilityId}
                   serviceRequestId={serviceRequestId}
                   setIsCompleteDialogOpen={setIsCompleteDialogOpen}
+                  onPendingChange={setIsCompletingServiceRequest}
                   onCancel={() => setIsCompleteDialogOpen(false)}
                   serviceRequest={request}
                 />
 ...
             <Dialog
               open={isCompleteDialogOpen}
-              onOpenChange={setIsCompleteDialogOpen}
+              onOpenChange={(open) => {
+                if (!isCompletingServiceRequest) setIsCompleteDialogOpen(open);
+              }}
             >
 interface CompletionNoteContentProps {
   facilityId: string;
   serviceRequestId: string;
   onCancel: () => void;
   setIsCompleteDialogOpen: (open: boolean) => void;
+  onPendingChange: (isPending: boolean) => void;
   serviceRequest: ServiceRequestReadSpec;
 }

 const CompletionNoteContent = ({
   facilityId,
   serviceRequestId,
   onCancel,
   setIsCompleteDialogOpen,
+  onPendingChange,
   serviceRequest,
 }: CompletionNoteContentProps) => {
 ...
   } = useMutation({
     mutationFn: mutate(serviceRequestApi.updateServiceRequest, {
       pathParams: { facilityId, serviceRequestId },
     }),
+    onMutate: () => {
+      onPendingChange(true);
+    },
     onSuccess: () => {
       toast.success(t("service_request_completed"));
       setIsCompleteDialogOpen(false);
       queryClient.invalidateQueries({
         queryKey: ["serviceRequest", facilityId, serviceRequestId],
       });
     },
+    onSettled: () => {
+      onPendingChange(false);
+    },
   });

Also applies to: 707-721

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx` around
lines 640 - 663, The Sheet and Dialog components allow external close events
(outside-click, Escape key, close button) to dismiss the completion surface via
onOpenChange={setIsCompleteDialogOpen}, even while the completion mutation is
pending inside CompletionNoteContent. To fix this, retrieve the pending state
from the CompletionNoteContent child component (where the mutation lives) and
propagate it upward to the parent. Then modify the onOpenChange handler to
conditionally prevent the dialog from closing by wrapping
setIsCompleteDialogOpen with logic that checks the pending state - if the
mutation is in flight, ignore the close request. This same fix should be applied
to both the Sheet component (around line 640) and the Dialog component (around
line 707).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx`:
- Around line 707-745: Add documentation above the useMutation hook for
completeServiceRequest that explains the medical data flow and validation
boundaries. Include a comment that describes what medical data is being
persisted (the completion note and location IDs), the validation that occurs
(such as the note being trimmed), and the data-flow path from the form input
through the mutation to the backend update. This follows the coding guideline
that page components in src/pages/**/*.{ts,tsx} must document medical data flows
and validation requirements.
- Around line 688-762: Extract the CompletionNoteContent component from
ServiceRequestShow.tsx into a new file at components/CompletionNoteContent.tsx
as a default export. Update the CompletionNoteContentProps interface to replace
the two separate callbacks onCancel and setIsCompleteDialogOpen with a single
onClose callback. Update all usages of these two callbacks within the component
to call onClose instead. This consolidation simplifies the component's interface
and follows the one-component-per-file convention with default exports as per
the coding guidelines.
- Around line 640-663: The Sheet and Dialog components allow external close
events (outside-click, Escape key, close button) to dismiss the completion
surface via onOpenChange={setIsCompleteDialogOpen}, even while the completion
mutation is pending inside CompletionNoteContent. To fix this, retrieve the
pending state from the CompletionNoteContent child component (where the mutation
lives) and propagate it upward to the parent. Then modify the onOpenChange
handler to conditionally prevent the dialog from closing by wrapping
setIsCompleteDialogOpen with logic that checks the pending state - if the
mutation is in flight, ignore the close request. This same fix should be applied
to both the Sheet component (around line 640) and the Dialog component (around
line 707).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 41c2daaa-2382-4c8b-ac66-90fe0a05a767

📥 Commits

Reviewing files that changed from the base of the PR and between 5280d07 and b3add71.

📒 Files selected for processing (1)
  • src/pages/Facility/services/serviceRequests/ServiceRequestShow.tsx

@github-actions

Copy link
Copy Markdown

🎭 Playwright Test Results

Status: ✅ Passed
Test Shards: 3

Metric Count
Total Tests 321
✅ Passed 321
❌ Failed 0
⏭️ Skipped 0

📊 Detailed results are available in the playwright-final-report artifact.

Run: #9592

@nihal467 nihal467 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolve all the AI review comments

@NikhilA8606 NikhilA8606 requested a review from nihal467 June 25, 2026 10:20
@NikhilA8606 NikhilA8606 changed the title ENG-581 Optimize complete service request logic by remove duplication of mutation [ENG-581] Optimize complete service request logic by remove duplication of mutation Jun 26, 2026
@NikhilA8606

NikhilA8606 commented Jun 26, 2026

Copy link
Copy Markdown
Member Author

ENG-581

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants